Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update the shipping time controllers to handle the maximum shipping time #2590

Merged
merged 5 commits into from
Sep 15, 2024
Merged

Update the shipping time controllers to handle the maximum shipping time #2590

merged 5 commits into from
Sep 15, 2024

Conversation

jorgemd24
Copy link
Contributor

@jorgemd24 jorgemd24 commented Sep 5, 2024 •
edited
Loading

Changes proposed in this Pull Request:

Part of pcTzPl-2qP-p2

This PR updates the shipping time controllers to allow for updating and fetching the maximum shipping time, which will be used in a second PR that I’m currently working on for the UI.

Screenshots:

Detailed test instructions:

Prerequisites

  1. Follow the steps mentioned in this PR to update the database: Add Shipping Max time column in gla_shipping_times Table #2520

  1. Make a GET /wc/gla/mc/shipping/times request and check that the max_time property is now included in the response.
  2. Update one of the shipping times by making the following request: POST gla/mc/shipping/times and the following body: {"country_code":"ES","time":2, "max_time": 3}
  3. Make the following request for one specific country: GET gla/mc/shipping/times/YOUR_COUNTRY for example GET gla/mc/shipping/times/ES and check that the max_time property is now included.

Additional details:

Changelog entry

@github-actions github-actions bot added the changelog: update Big changes to something that wasn't broken. label Sep 5, 2024
@jorgemd24 jorgemd24 changed the base branch from update/shippings-settings-phase-1 to develop September 5, 2024 20:04
@jorgemd24 jorgemd24 changed the base branch from develop to update/shippings-settings-phase-1 September 5, 2024 20:04
@codecov Codecov
Copy link

codecov bot commented Sep 5, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 77.50000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 66.3%. Comparing base (8f3231f) to head (af40ca3).
Report is 148 commits behind head on update/shippings-settings-phase-1.

Files with missing lines Patch % Lines
...trollers/MerchantCenter/ShippingTimeController.php 85.3% 5 Missing ⚠️
src/DB/Table/ShippingTimeTable.php 0.0% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                          Coverage Diff                          @@
##             update/shippings-settings-phase-1   #2590     +/-   ##
=====================================================================
+ Coverage                                 65.2%   66.3%   +1.0%     
- Complexity                                4588    4598     +10     
=====================================================================
  Files                                      803     475    -328     
  Lines                                    23012   17927   -5085     
  Branches                                  1234       0   -1234     
=====================================================================
- Hits                                     15012   11879   -3133     
+ Misses                                    7833    6048   -1785     
+ Partials                                   167       0    -167
Flag Coverage Δ
js-unit-tests ?
php-unit-tests 66.3% <77.5%> (+0.7%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ers/MerchantCenter/ShippingTimeBatchController.php 92.5% <100.0%> (+0.4%) ⬆️
src/DB/Table/ShippingTimeTable.php 50.0% <0.0%> (-3.8%) ⬇️
...trollers/MerchantCenter/ShippingTimeController.php 93.6% <85.3%> (+53.4%) ⬆️

... and 327 files with indirect coverage changes

@jorgemd24 jorgemd24 marked this pull request as ready for review September 5, 2024 20:23
@jorgemd24 jorgemd24 requested a review from a team September 5, 2024 20:24
@jorgemd24 jorgemd24 changed the title Update/shipping time controllers to work with max time Update the shipping time controllers to handle the maximum shipping time Sep 5, 2024
@puntope
Copy link
Contributor

puntope commented Sep 9, 2024

@jorgemd24 I see one potential issue in the controller where you can set -1 as value for time and 'max_time':

  1. Request POST wp-json/wc/gla/mc/shipping/times with {"country_code":"es, "max_time": -1, time: -1 }
  2. Request is successful
  3. Request GET wp-json/wc/gla/mc/shipping/times/es
  4. It returns max_time and time as -1
Screenshot 2024-09-09 at 11 24 58 Screenshot 2024-09-09 at 11 24 51

@puntope
Copy link
Contributor

puntope commented Sep 9, 2024 •
edited
Loading

Another potential issue I see is that we don't require time and max_time which is overrides as 0 is you don't set them.

  1. Request POST wp-json/wc/gla/mc/shipping/times with {"country_code":"es, "max_time": 1, time: 1 }
  2. Request is successful
  3. Request GET wp-json/wc/gla/mc/shipping/times/es
  4. It returns max_time and time as 1
  5. Request POST wp-json/wc/gla/mc/shipping/times with {"country_code":"es, "max_time": 2 }
  6. Request is successful
  7. Request GET wp-json/wc/gla/mc/shipping/times/es
  8. It returns max_time 2 but time as 0 (it shouldn't be 1?)

@puntope
Copy link
Contributor

puntope commented Sep 9, 2024

Finally, one extra issue I see is that time can be set as a bigger number that max_time

  1. Request POST wp-json/wc/gla/mc/shipping/times with {"country_code":"es, "max_time": 1, time: 2 }
  2. Request is successful
  3. Request GET wp-json/wc/gla/mc/shipping/times/es
  4. It returns max_time 1 and time as 2
Screenshot 2024-09-09 at 11 39 21 Screenshot 2024-09-09 at 11 37 39

@jorgemd24
Copy link
Contributor Author

Thanks, @puntope, for your helpful review! I've made the changes based on your comments. Could you please take another look? Thanks!

puntope
puntope reviewed Sep 13, 2024
View reviewed changes
src/API/Site/Controllers/MerchantCenter/ShippingTimeController.php
Comment on lines +290 to +295
protected function get_args_schema(): array {
$schema = $this->get_schema_properties();
$schema['time']['required'] = true;
$schema['max_time']['required'] = true;
return $schema;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious. Why we cannot just add the 'required' in the array inside get_schema_properties method ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my initial approach, but the schema is shared between the GET and POST methods. If I make time and max_time required, the UI will also need to include those query parameters when fetching the times.

puntope
puntope approved these changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@puntope puntope left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jorgemd24 for the adjustments validating the request.

Now LGTM

I left just one question regarding where we place the required arg.

@jorgemd24 jorgemd24 merged commit 8ffaa67 into update/shippings-settings-phase-1 Sep 15, 2024
12 checks passed
@jorgemd24 jorgemd24 deleted the update/shipping-time-controllers-to-work-with-max-time branch September 15, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@puntope puntope puntope approved these changes

Assignees

@jorgemd24 jorgemd24

Labels
changelog: update Big changes to something that wasn't broken.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jorgemd24 @puntope